Skip to content

Quote {build.source.path} to allow spaces in path #4868

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 26, 2021

Conversation

tttapa
Copy link
Contributor

@tttapa tttapa commented Feb 28, 2021

Previously sketches or examples that had spaces anywhere in their absolute path caused a total build failure. By adding quotes around the path in platform.txt, they now build correctly.

Consider a “bare minimum” sketch in /home/pieter/Arduino/folder with spaces/Sketch/Sketch.ino. Previously, the bash command generated for copying the partitions.csv file was:

bash -c "[ ! -f /home/pieter/Arduino/folder with spaces/Sketch/partitions.csv ] || cp -f /home/pieter/Arduino/folder with spaces/Sketch/partitions.csv /tmp/arduino_build_324565/partitions.csv"
bash: line 0: [: too many arguments
cp: target '/tmp/arduino_build_324565/partitions.csv' is not a directory
exit status 1

With the proposed changes, the path in the bash command is now correctly quoted, and the build succeeds:

bash -c "[ ! -f \"/home/pieter/Arduino/folder with spaces/Sketch\"/partitions.csv ] || cp -f \"/home/pieter/Arduino/folder with spaces/Sketch\"/partitions.csv /tmp/arduino_build_324565/partitions.csv"

This pull request addresses issues #4579 and #4640.

People use spaces in folder names, and this works correctly in version 1.0.4 of this ESP32 core and all other Arduino cores I've tried. It no longer works in version 1.0.5 (after commit 9ef3e2d), so I believe this is well worth fixing, especially since it's such a simple change.

Previously sketches or examples that had spaces anywhere in their absolute
path caused a total build failure. By adding quotes around the path in
platform.txt, they now build correctly
@me-no-dev
Copy link
Member

quotation does not look alright. this needs testing :)

@tttapa
Copy link
Contributor Author

tttapa commented Mar 1, 2021

The confusing thing about the quotation is that the Arduino IDE seems to handle quotes differently when they are next to a placeholder:

"outer string "inner string {placeholder} more inner string" more outer string"

gets converted into

"outer string "inner string value more inner string" more outer string"

which is obviously incorrect. On the other hand,

"outer string "{placeholder}" more outer string"

becomes

"outer string \"value\" more outer string"

which is correct in this case.

This is kind of uncommon, though, and I couldn't find any mention of this behavior in the platform specification docs.

I've tested this using the Arduino IDE v1.8.9 and 1.8.13 on Ubuntu 20.04, as well as with the arduino-cli directly for my CI builds and it works correctly. The bash commands it generates look correct to me (enable verbose output in the preferences to see them, the output is also in my previous message) and the partitions.csv file gets copied to the build folder if it exists, no matter if the sketch path contains spaces or not.

@me-no-dev
Copy link
Member

what if we pre-escape the whole paths so that partitions.csv is also escaped. I also need to test this with Sloeber and make sure that paths expand as they should

@tttapa
Copy link
Contributor Author

tttapa commented Mar 2, 2021

Why does partitions.csv need to be escaped? It doesn't contain any spaces or special characters.

Bash should handle this correctly, multiple strings (quoted or not quoted) that are right next to each other are combined into a single string.

For example:

$ echo "quoted&:'string"suffix
> quoted&:'stringsuffix
$ ls "$HOME"/.profile
> /home/pieter/.profile

@me-no-dev
Copy link
Member

because the commands are used by other builders and IDEs. It makes most sense that the full path is escaped, not just part of it.

@me-no-dev me-no-dev mentioned this pull request Mar 26, 2021
@me-no-dev me-no-dev merged commit 46d5afb into espressif:master Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants